test: add StartableToolSet backoff regression tests and docs - #4065
test: add StartableToolSet backoff regression tests and docs#4065aheritier wants to merge 2 commits into
Conversation
…rt, align bounds Addresses all blocking and should-fix findings from the aheritier review on PR #4062: [blocking #1 + #2] Generic classifier with HTTP-status precedence: - Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a retryable HTTP status (429/408/5xx) via *StatusError or message regex, without string-pattern heuristics ('connection refused' stays non-retryable). - startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err). A StatusError{429} coexisting with context.DeadlineExceeded now arms the gate (HTTP wins), fixing the deadline-masks-rate-limit race. [blocking #3] Bounds aligned with remediation plan: - base = 15s, cap = 5min (was 1s/30s). - Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the full nominal wait is always respected. [blocking #4] Gate enforced only in the TryStart path: - Move gate check from startLocked into new tryStartLocked (called by TryStart/TryStartWithTimeout only). - Start() calls startLocked directly — mcpcatalog enable and skill sub-session startup are never delayed. [should-fix #5] External recovery via StartReporter: - tryStartLocked checks reporter.IsStarted() when started==false; a live reporter (e.g. after /toolset-restart) clears the gate and latches the wrapper without calling the underlying Start. - New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate. [should-fix #6] Exported constructor options for cross-package tests: - NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter, WithStartRetryClock. - nowFn() clock seam; zero-value StartableToolSet still usable. [should-fix #7] Concurrent and at-boundary tests: - TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines calling TryStart, assert underlying Start invoked exactly once. - TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry. [optional] Stale comment name in BackoffDormantForPlainErrors fixed. Also: - RetryableHTTPStatus test cases include plain-text regex fallback. - ExportedSetClock removed (unused; WithStartRetryClock preferred). - All gating tests converted from s.Start() to s.TryStart(). - Jitter-bounds assertions updated to [nominal, 1.2×nominal]. PR2 (#4065) will need rebasing and test updates after this lands.
Review: needs work before this can landCOMMENT only — not approvable yet. This PR is still a draft, Go CI has not run for this stacked branch, and the test/docs changes are out of sync with the current #4062 implementation. The test approach has genuine value—especially the consumer-shaped fakes, CI status: Go CI did not runOnly documentation workflows ran and passed. This PR targets Therefore, [blocking] The stack is stale; five tests fail against the current #4062 headThis PR is based on the first #4062 commit ( Those changes altered the exact contracts encoded here:
Rebasing onto the current #4062 implementation produces failures in:
Please rebase this PR before refining the test expectations. The failures are behavioral contract drift, not build or lint failures. [blocking] The tests exercise
|
…rt, align bounds Addresses all blocking and should-fix findings from the aheritier review on PR #4062: [blocking #1 + #2] Generic classifier with HTTP-status precedence: - Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a retryable HTTP status (429/408/5xx) via *StatusError or message regex, without string-pattern heuristics ('connection refused' stays non-retryable). - startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err). A StatusError{429} coexisting with context.DeadlineExceeded now arms the gate (HTTP wins), fixing the deadline-masks-rate-limit race. [blocking #3] Bounds aligned with remediation plan: - base = 15s, cap = 5min (was 1s/30s). - Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the full nominal wait is always respected. [blocking #4] Gate enforced only in the TryStart path: - Move gate check from startLocked into new tryStartLocked (called by TryStart/TryStartWithTimeout only). - Start() calls startLocked directly — mcpcatalog enable and skill sub-session startup are never delayed. [should-fix #5] External recovery via StartReporter: - tryStartLocked checks reporter.IsStarted() when started==false; a live reporter (e.g. after /toolset-restart) clears the gate and latches the wrapper without calling the underlying Start. - New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate. [should-fix #6] Exported constructor options for cross-package tests: - NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter, WithStartRetryClock. - nowFn() clock seam; zero-value StartableToolSet still usable. [should-fix #7] Concurrent and at-boundary tests: - TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines calling TryStart, assert underlying Start invoked exactly once. - TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry. [optional] Stale comment name in BackoffDormantForPlainErrors fixed. Also: - RetryableHTTPStatus test cases include plain-text regex fallback. - ExportedSetClock removed (unused; WithStartRetryClock preferred). - All gating tests converted from s.Start() to s.TryStart(). - Jitter-bounds assertions updated to [nominal, 1.2×nominal]. PR2 (#4065) will need rebasing and test updates after this lands.
2ef2ff5 to
0ab3683
Compare
Fixes issue #4060: RAG semantic-embeddings indexing triggered a rate-limit retry storm because repeated toolset-start attempts had no pacing — every agentic loop iteration re-entered Manager.Initialize at full speed after a 429 from the embedding provider. Implementation: - Add modelerrors.RetryableHTTPStatus(err): HTTP-status classifier that recognises 429, 408, and 5xx signals via *StatusError (no regex fallback, so port numbers / chunk counters cannot trigger backoff) - Add pkg/tools/startable_backoff.go: bounded exponential backoff with additive 0-20% jitter (base=15s, cap=5min, delay∈[d,1.2d]) that de-synchronises concurrent toolset sources and guarantees the full nominal wait as a floor - Wire gate into tryStartLocked (called only by TryStart/ TryStartWithTimeout): blocking Start() bypasses the gate so mcpcatalog enable and skill sub-session startup remain immediate - Gate also detects live StartReporter after a /toolset-restart and adopts the recovery without waiting for the window to expire - Add WithStartRetryJitter / WithStartRetryClock constructor options (NewStartable variadic) for deterministic test control - Update stale 'retry on next turn' log messages in agent.go / mcp.go - Document the partial-start exemption (code-mode composites remain unpaced; tracked at issue #4067) Scope: DefaultStartTimeout (30s) is unchanged — deferred.
Regression suite and documentation for the backoff gate introduced in the preceding commit (issue #4060). Tests (no production code changes): - pkg/tools/startable_backoff_regression_test.go: consumer-shaped regression scenarios verifying gate behaviour for RAG (429-wrapped StatusError), MCP/LSP compatibility (plain errors fail fast, no gate), blocking Start() is never gated, concurrent TryStart calls invoke the underlying exactly once, no timers/goroutines leaked, context cancellation sets no window, additive jitter de-synchronises retries ([d,1.2d] bounds with spread assertion), latch semantics preserved, HTTP 408 gate. - pkg/tools/builtin/rag/rag_backoff_test.go: real-toolset integration tests using rag.New + countingStatusErrStrategy — StatusError(429) gates TryStart via WithStartRetryClock fake clock; plain assert.AnError fails fast. Docs: - docs/tools/rag/index.md: new section 'Indexing failures, retries and backoff' — trigger table (429/408/5xx vs fail-fast), parameters (15s base, 5min cap, additive jitter [d,1.2d]), before/after impact, troubleshooting (max_indexing/embedding_concurrency knobs). - docs/tools/mcp/index.md, docs/tools/lsp/index.md: short lifecycle notes confirming local startup failures fail fast; cross-link to RAG section.
5d47f6c to
880ea85
Compare
Folded into #4062 — test and documentation commit cherry-picked directly onto the fix branch. All content from this PR (regression tests, RAG integration tests, and docs) is now at HEAD |
Depends on PR #4062 (stacked). Base retargets to
mainonce PR1 merges.Regression suite and documentation for the backoff gate added in PR1.
Tests (no production code changes):
pkg/tools/startable_backoff_regression_test.go— 9 consumer-shaped scenarios: RAG-shaped failure→backoff→recovery (with realistic two-level wrapping), HTTP 408 gate, MCP/LSP compatibility (fail-fast, unchanged), concurrent starts don't multiply retries, no timer/goroutine leak (synctest settling), cancellation no-window, jitter de-sync, already-started not restartedpkg/tools/builtin/rag/rag_backoff_test.go— 2 real-toolset tests usingrag.New+ a counting strategy: StatusError(429) → gate fires, plain error → fail-fastDocs:
docs/tools/rag/index.md— new## Indexing failures, retries and backoffsection: trigger table (429, 408, 5xx backoff; other 4xx/cancellation fail-fast), parameters (1s/30s/equal-jitter), before/after impact, troubleshootingdocs/tools/mcp/index.md+docs/tools/lsp/index.md— short lifecycle notes clarifying local failures fail fast, cross-linking to RAG sectionKey architectural note: the conservative classifier (PR1) means only RAG embedding calls surface structured StatusErrors in practice. MCP and LSP local failures fail fast — their behavior is unchanged, which this PR's compatibility tests lock in.